I think this changes the semantics around reset in a way that would affect other valid use cases. Consider when the intention is to reset a timer that has not finished yet. In that case I think you would want it to return to zero.

Maybe adding a new method is called for with a name like: resume, continue or rollover ?

Tested it with a resettable timer and you're totally right! Fixed in the next commit.

Not sure what the added value would be of an extra method. Both cases can be covered by a single method as far as I can tell.

I suggested a new method because reset to me, sounds like it should always zero out the timer.

The additional change you added to handle reset before a timer is finished improves the situation but I think there are still situations where this automatic carry over would be surprising.

What about a timer that is only reset in order to measure an infrequent event (not looping all the time). Imagine a timer used to leave an informational message on the screen for a specified interval. It would be surprising if the timer carried over the previous elapsed time when it was reset. I suppose if it were an infrequent timer like that, it wouldn't have a significant effect if the timer was only carried forward by a single run loop's worth of elapsed time. But as it's currently implemented it could go along ticking the elapsed variable for a long duration and by the time reset is called it would be at some arbitrary value.

That made me think a new method or a new type like LoopingTimer might be nice.

If we want to make this support both looping with carry over, as well as infrequent event time measurements I think a little bit more logic in tick() might get us there.

How about something like this:

//...
    pub fn tick(&mut self, delta: f32) {
        if self.finished {
            // A tick after already finished before `reset` was called indicates that carry over
            // of elapsed time for a looping timer was not intended.
            self.elapsed = self.duration;
            return;
        }
        self.elapsed = self.elapsed + delta;
        if self.elapsed >= self.duration {
            self.finished = true;
        }
    }

    pub fn reset(&mut self) {
        self.elapsed = if self.finished {
            self.elapsed % self.duration
        } else {
            0.0
        };
        self.finished = false;
    }
//...

Here are some tests for the above:

#[cfg(test)]
mod tests {
    use super::{Timer};

    #[test]
    fn test_carry_over() {
        let mut timer = Timer::from_seconds(3.0);
        timer.tick(3.5);
        assert!(timer.finished == true);
        timer.reset();
        // Carry over because the timer was finished during the most recent tick.
        assert_eq!(timer.elapsed, 0.5);
    }

    #[test]
    fn test_carry_over_not_applied_to_unfinished_timer() {
        let mut timer = Timer::from_seconds(3.0);
        timer.tick(2.5);
        assert!(timer.finished == false);
        timer.reset();
        // No carry over since timer wasn't finished.
        assert_eq!(timer.elapsed, 0.0);
    }

    #[test]
    fn test_carry_over_not_applied_to_rarely_reset_timer() {
        let mut timer = Timer::from_seconds(3.0);
        timer.tick(2.5);
        assert!(timer.finished == false);
        timer.tick(2.5);
        assert!(timer.finished == true);
        assert_eq!(timer.elapsed, 5.0);

        // Tick again after finished without reseting means the timer is set to `duration`
        timer.tick(2.5);
        assert_eq!(timer.elapsed, 3.0);

        timer.reset();
        // No carry over since timer wasn't finished after the most recent tick.
        assert_eq!(timer.elapsed, 0.0);
    }
}

running 3 tests
test tests::test_carry_over_not_applied_to_unfinished_timer ... ok
test tests::test_carry_over_not_applied_to_rarely_reset_timer ... ok
test tests::test_carry_over ... ok

Thanks for the explanation! That does clear it up a lot for me.

Given the example used in the introduction (https://bevyengine.org/learn/book/getting-started/resources/) I had the impression Timer was supposed to be a repeating construct from the get go, but the case you've pointed out makes a lot of sense too.

Maybe adding a repeating flag in the constructors would give the most intuitive behavior. In non repeating mode the timer stops after finishing once, then reset() should be called to re-use the timer, setting the elapsed time back to 0.0. In repeating mode the timer sets the finished state during the update in which the duration was reached and automatically resets it's elapsed time carrying over any excess. In the next update finished should then be false again. This also seems more ergonimical since for repeating timers you don't have to keep calling a reset/resume function manually; you just set it to go and keep check of whether or not it finished.

As an example of the above, I've yet to actually test this:

    pub fn tick(&mut self, delta: f32) {
        match (self.repeating, self.elapsed >= self.duration) {
            // non repeating timer
            (false, false) => {
                self.elapsed = self.elapsed + delta;
            },
            (false, true) => {
                self.finished = true;
            },

            // repeating timer
            (true, false) => {
                self.elapsed = self.elapsed + delta;
                self.finished = false;
            }
            (true, true) => {
                self.elapsed = self.elapsed % self.duration;
                self.finished = true;
            }
        }
    }

    pub fn reset(&mut self) {
        self.finished = false;
        self.elapsed = 0.0;
    }

Edit: Added an implementation for this.

By removing this if check, elapsed could be used to calculate how long ago did the timer finish + a minor perf boost.
Or is there a concern for the float to overflow?

I think it's more about expected behavior; if I set a timer for 10 seconds I expect it to notify me after 10 seconds and stop doing anything after that, but maybe that's just me.

I did have overflow in mind adding the check but I didn't check how long you'd actually have to leave it running. Since it's going to run out at some point anyways you're probably better off storing an Instant externally and comparing that later on.

what this field doesn't isn't immediately obvious, could you document it?

Added some comments to prev_finished and just_finished() to clarify.